Thanks @maukoquiroga.

This is marked as depending on #1033 but I see redundant changes, for example in the config file.

I would be in favour of having a single PR as they seem to address the same issue (openfisca/openfisca-doc#244) and to improve documentation altogether.
If you'd prefer not to for some reason, could we at least update the target branch to #1033? 🙂 The current changesets seem likely to yield conflicts.

This is marked as depending on #1033 but I see redundant changes, for example in the config file.

Corrected it to depend on #1038 to depend on both #1038 and #1033, and changed base for clarity as suggested , the redundant changes are effectively just in the setup and config files, the rest has to be properly rebased as they're independent from #1033 (I will do shortly).

If you'd prefer not to for some reason, could we at least update the target branch to #1033? 🙂 The current changesets seem likely to yield conflicts.

I will rebase properly, that should get us rid of the conflicts 😃

I would be in favour of having a single PR as they seem to address the same issue (openfisca/openfisca-doc#244) and to improve documentation altogether.

I thought so initially as well, and I tried, then I decided to split this work by "submodule" and add doc as well as fixing the doctests. Here's what happened:

• I tried to fix all doctests at once, which I couldn't for several reasons:

  • Some parts of the code are just un-unit-testable without actual refactoring (creating a Holder for example involves almost half of the codebase, including the Holder itself…)
  • Then I tried decoupling some classes to use dependency injection —even if that means breaking changes— as so to make them more granular an unit-testable, but the actual test suite do not cover all scenarios, catch-22…
  • In a last effort to try to understand what I was doing I turned myself over to the documentation, which is excellent already, but still incomplete and outdated, because not checked regularly and automatically, catch-22…

• Then I adopted the current attack strategy so to:

  • Fix/complete the doctests of the more independent/uncoupled modules, to avoid (yet) refactoring
  • Fix/complete the documentation to discover refactoring opportunities (the todos)
  • To set a standard that can be applied partially but progressively over time
  • To increase the confidence on the tests enough as to do the needed refactoring and breaking changes

I think we probably won't be able to fix all the doctests soon, that's why I changed from an horizontal to a vertical (submodules) approach.

Happy to have an extended discussion on that with the community if you think that could be useful!

@MattiSG @sandcha @benjello ?

I am in favor of more typing, more doctest but I am ,ot sure I can grasp all the implications of the changes ...

The added documentation is great! However, I don't understand the consequences of introducing an abstract base class system

Thanks @MattiSG !

Concerning the abstract class system, it is not one. As I've described it:

This PR consolidates the notion of protocols, or duck-typing, or structural sub-typing: that means type-checks are done not against the actual implementation of a model, but against a protocol, that is, the equivalent of an interface, or an abstract model, but without impact at runtime.

Structural sub-typing is described in PEP 544.

Quoting it:

At runtime, protocol classes will be simple ABCs. There is no intent to provide sophisticated runtime instance and class checks against protocol classes. (...) Protocols are completely optional:

• No runtime semantics will be imposed for variables or parameters annotated with a protocol class.
• Any checks will be performed only by third-party type checkers and other tools.

In other words, there are no consequences beyond type-checks —no runtime impact then.

To make the point, an Entity is to an EntityProtocol the same a List is to a Sequence: instead of type-checking for implementation, we type-check for behaviour (that is why it is nicknames "duck-typing").

This has, however, one major benefit: we remove circular module dependencies from the codebase, which has an actual positive impact in terms of modularity and testability.

and this “variable proxy” system, so I cannot approve that PR 😕

That is just an abstraction, or in DDD, a port.

As I see it has two impacts:

• It adds explicitness —we encapsulate the relationship between entities and variables and make it explicit
• It adds modularity —again it helps decoupling models and making a clear distinction between data/logic

Beyond the "design" aspect of it, the actual recipe, the descriptor, is just standard Python —a huge deal of the function system, properties, and so on, are just syntactic sugar; behind-the-scenes they're just descriptors.

Finally, as you can see from the changeset, the introduced syntax is more coherent with the projector system used everywhere in OpenFisca, which should, sooner rather than later, IMHO, be reimplemented as a descriptor as well.

Superseded by #1255